fix: use the same background executor in otel reader and monitoring c… #2746
Conversation
product-auto-label
bot
added
size: m
|
|
||
| settingsBuilder.setUniverseDomain(universeDomain); | ||
|
|
||
| if (executorService != null) { |
There was a problem hiding this comment.
when is this null? add a comment
There was a problem hiding this comment.
The comment doesnt explain why executorService would be null
| * endpoint. | ||
| */ | ||
| public static void registerBuiltinMetrics( | ||
| @Nullable Credentials credentials, SdkMeterProviderBuilder builder, @Nullable String endpoint) |
There was a problem hiding this comment.
should this method be deprecated?
There was a problem hiding this comment.
I don't think so, we just marked everything else with project id as deprecated since project id is not used anymore. Also the whole class is deprecated because we want to encourage poeple to move to CustomOpenTelemetryMetricsProvider
igorbernstein2
left a comment
igorbernstein2
left a comment
There was a problem hiding this comment.
lgtm after comments are addressed
| // if the background executor in the settings is already a FixedExecutorProvider, | ||
| // we can't assume that we can autoclose it and the life cycle should be managed | ||
| // by the application | ||
| canAutoCloseExecutor = false; |
There was a problem hiding this comment.
Can we create our own implementation of the ExecutorProvider with a settable bit for shouldAutoClose and use that instead of the FixedExecutorProvider? This way we can localize the lifecycle management instead of messing around with background resources?
But still passing a FixedExecutorProvider to the child clients like metrics?
2 participants
…lient
Use the same background executor in monitoring client and OpenTelemetry Periodic readers as the bigtable client. This executor is only used for background tasks, so monitoring client is still gonna have different executors for its channels.
Since we convert the background executor to a fixed executor provider, we need to add it back to the background resources to auto close it.